Add remaining 3 phase electrical attributes #339
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #339 +/- ##
==========================================
+ Coverage 96.51% 96.52% +0.01%
==========================================
Files 61 61
Lines 9457 9501 +44
==========================================
+ Hits 9127 9171 +44
Misses 330 330 ☔ View full report in Codecov by Sentry. |
| @@ -126,7 +170,6 @@ class MeasurementType(enum.IntFlag): | |||
| ElectricalMeasurement.AttributeDefs.measurement_type.name: True, | |||
| ElectricalMeasurement.AttributeDefs.power_divisor.name: True, | |||
| ElectricalMeasurement.AttributeDefs.power_multiplier.name: True, | |||
| ElectricalMeasurement.AttributeDefs.power_factor.name: True, | |||
There was a problem hiding this comment.
I am not sure why power_factor was here, but I think reporting needs to be configured for it too
There was a problem hiding this comment.
According to ZCL spec, it is not reportable.
There was a problem hiding this comment.
Hmm, apparently we also set up attribute reporting for some attributes that don't apparently don't even support it before this PR...
There was a problem hiding this comment.
Removed reporting for all the ones that are unsupported
There was a problem hiding this comment.
So the "good thing" is that we already poll all supported attributes at runtime, so that's why power_factor and others work in the first place.
But for the ones that change over time, where attribute reporting can't be set up (according to spec), we should just add them to the ZCL_INIT_ATTRS dict with cache=True, as we don't need to poll them when starting up ZHA (the unsupported_attributes check isn't done there), but we do need to poll them at runtime, which is done through the cluster handler, so there is one big attribute read for multiple attributes.
But we should still move some attributes (where attribute reporting isn't possible) from REPORT_CONFIG to ZCL_INIT_ATTRS.
(EDIT: Wrote this before your comment above 😄)
There was a problem hiding this comment.
It seems that we only poll attributes that for which we set up reporting, because we only poll the ones in REPORT_CONFIG, so if we move them to ZCL_INIT_ATTRS they will not be polled, right?
Also, why do we poll the ones that have reporting set up?
There was a problem hiding this comment.
Ah, you're right. I confused the startup polling doing both and this one only doing the reporting attributes.
We poll the attribute because many smart plugs (mostly Tuya) do not respect/allow setting up attribute reporting on most/all EM cluster attributes.
I do wonder a bit how some of this would work at all then, or at least how it works at the moment (with power_factor and so on), but maybe we've just not had any devices that support it? I wanna check how Z2M deals with some of these devices later. It has a polling configuration per plug/device at least.
There was a problem hiding this comment.
Either no one complained about power_factor not getting updated (since there maybe not be many devices with it), or the devices report the value even without reporting config 🤷
I reverted the attributes back to REPORT_CONFIG for now.
| ElectricalMeasurement.AttributeDefs.power_factor.name: False, | ||
| ElectricalMeasurement.AttributeDefs.power_factor_ph_b.name: False, | ||
| ElectricalMeasurement.AttributeDefs.power_factor_ph_c.name: False, |
There was a problem hiding this comment.
Per this comment, we should be able to use True for the cache here.
They are polled using the EM cluster at runtime, which checks for unsupported_attributes, so we can avoid the extra polling during during each startup, which doesn't check for unsupported_attributes (yet).
Even if we eventually add it, we can avoid polling this at startup, since we'll poll it after a few seconds/minute anyway.
EDIT: The polling part is not completely correct: #339 (comment)
|
Instead of trying to set up attribute reporting for attributes that aren't reportable according to spec, just so the EM cluster will poll them with the current implementation, we could also add another Then, we'd have all attributes that need to be initialized in
Another thing, do we really need to poll the |
In that case, won't all the attributes in
To be honest I don't know. Since they were already in |
Yeah, probably all attributes that are in If it looks really stupid to duplicate most/all attributes, we can also combine them in |
As suggested in zigpy#339 (comment) this adds an `ZCL_POLLING_ATTRS` to define attributes that should be polled separatelly from the ones that can have reporting config. This also moves some attributes that do no support reporting config out of `REPORT_CONFIG`.
|
Opened a PR with the suggested changes: #354 |
Follow up to #324
Related HA PR: home-assistant/core#133969